Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#3 - User Authentication #18

Merged
merged 34 commits into from
Dec 12, 2024
Merged

#3 - User Authentication #18

merged 34 commits into from
Dec 12, 2024

Conversation

Silabalkan
Copy link
Collaborator

@Silabalkan Silabalkan commented Dec 2, 2024

Implemented user authentication using Laravel Breeze.
Integrated login and registration functionality.
Resolved conflicts with the authentication code.
Connected the authentication features with the Vue.js and Inertia.js front end.
This should close #3
This should close #4
This should close #5

Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please resolve conflicts
  2. Please change pr title - we have a rule that pr title should look like this:

#issue-number - title

So in this case, we should give title like
#3 - user auth

And also please remember that in pr description we should write down every issue that is connected with pr or any issues that should bo closed after merging this pr. Something like: This should close #issue-number

  1. Please don't push .env file

@Silabalkan Silabalkan changed the title Silabalkan user registration #3 - User Authentication Dec 2, 2024
senagokhan
senagokhan previously approved these changes Dec 2, 2024
Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks are failed - please fix this

.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please fix all warnings from GitHub - like this

image

  1. At our company, we avoid leaving comments in the code unless absolutely necessary. We believe that code should be self-explanatory. That said, if you really feel a comment is needed, don’t worry—we can live with it. 😄 Let’s just keep it to a minimum.

I know that most of comments are from Laravel Breeze and fresh Laravel installation, but I think we can remove them

  1. I think that tasks User login #4 and Password reset feature #5 can be closed because in Laravel Breeze out of the box we have features like login to application and reset password

  2. For me reset password doesn't work, i got an error like

The "" scheme is not supported; supported schemes for mailer "smtp" are: "smtp", "smtps".

I read that is problem with some composer dependency, so if you have the same problem, run composer require "symfony/mailer:~7.1.0" and push this - it should work

vite.config.js Outdated Show resolved Hide resolved
resources/js/bootstrap.ts Outdated Show resolved Hide resolved
vite.config.js Outdated Show resolved Hide resolved
Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Check is failed - please run make fix - it should fix all errors
  2. Extra blank lines have been unnecessarily added at the end of the file, resulting in two blank lines. This clutters the code and doesn't follow standard formatting practices. Please keep only one blank line at the end.

package.json Show resolved Hide resolved
resources/js/Pages/Auth/ForgotPassword.vue Outdated Show resolved Hide resolved
resources/js/Pages/Auth/Login.vue Outdated Show resolved Hide resolved
@EwelinaSkrzypacz
Copy link
Member

I think that tasks #4 and #5 can be closed because in Laravel Breeze out of the box we have features like login to application and reset password - so please write in pr description "This should close " and task number.

Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix GitHub errors and warnings in resources/js/Pages/Auth/VerifyEmail.vue. It's just some on them 👇

image

You can use make fix to fix errors, but this command sometimes doesn't fix all and errors must be fixed manually.

resources/js/app.ts Outdated Show resolved Hide resolved
@Silabalkan Silabalkan merged commit b347ec8 into main Dec 12, 2024
3 checks passed
@Silabalkan Silabalkan deleted the Silabalkan-user-registration branch December 12, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password reset feature User login User registration
4 participants